esm: add experimental support for addon modules #55844
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
legendecas
force-pushed
the
esm/addon
branch
from
d4b9a9a to
b384e80
Compare
legendecas
force-pushed
the
esm/addon
branch
3 times, most recently
from
c6ce9fd to
0c724cd
Compare
|
The commit message does not meet our guidelines, the word after the subsystem should be an infinitive verb. I suggest |
legendecas
changed the title
legendecas
changed the title
legendecas
force-pushed
the
esm/addon
branch
2 times, most recently
from
9841645 to
829cfc6
Compare
legendecas
force-pushed
the
esm/addon
branch
from
829cfc6 to
2ea455d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55844 +/- ##
==========================================
- Coverage 88.55% 88.54% -0.02%
==========================================
Files 657 657
Lines 190295 190377 +82
Branches 36542 36552 +10
==========================================
+ Hits 168511 168564 +53
- Misses 14969 14991 +22
- Partials 6815 6822 +7
|
github-actions
bot
removed
the
request-ci
|
|
||
| export async function run() { | ||
| // binding.node | ||
| { |
There was a problem hiding this comment.
Please use describe + it
There was a problem hiding this comment.
I personally find describe + it to be worse UX due to the reasons stated in #56027 (comment), unless there is a way to silence the passing tests and make the console logs from the failed test cases appear at the end together with the errors..
There was a problem hiding this comment.
Some suggestions, but non-blocking.
Addon export names can not be inferred until it is evaluated.
I wonder if there's anything we can do about it...potentially, defining some macro in node.h that the addons can use to export names with a specific pattern, which we can scan for by parsing the dynamic library (ideally through some new API in libuv, though we are already parsing binaries using postject for SEA which implements the parsing by itself...). Can be done as a follow-up, of course..
| @@ -23,6 +24,10 @@ if (experimentalWasmModules) { | |||
| extensionFormatMap['.wasm'] = 'wasm'; | |||
| } | |||
|
|
|||
| if (experimentalAddonModules) { | |||
| extensionFormatMap['.node'] = 'addon'; | |||
There was a problem hiding this comment.
I am somewhat surprised to find that there is not a test for the node-addons exports condition...it seems the the files pointed to by node-addons won't get interpreted as addons unless they have the .node extension, though this issue predates this PR and also happens to require().
There was a problem hiding this comment.
Taking a step back, I'm wondering if the format name could be renamed as node-addon/node-addons for alignment.
There was a problem hiding this comment.
Sure, also I am not even sure if anyone is actually using it, it seems only useful to distinguish "if the current run times can load Node.js style addons" as it currently stands, so may not really be that big of a deal anyway....
There was a problem hiding this comment.
I prefer the more concise addon
There was a problem hiding this comment.
Actually I think I misread #55844 (comment) - I think for the format name, addon or node-addon would make more sense. node-addons would be a bit weird in the hooks since it's plural.
There was a problem hiding this comment.
This loader format enum is specific for Node.js so I'll stick with the concise addon.
There was a problem hiding this comment.
In any case, the exports condition question is separate - it's more about when a module that doesn't end with .node is pointed to by node-addons, should it be interpreted(translated) into an addon or should it be interpreted based on whatever extension it has - and if it has a different extension (e.g. dylib or dll), what should happen then. As I said in the first comment, it's fine to keep it as is since CJS also only considers .node, but it is a bit strange for the exports condition.
There was a problem hiding this comment.
The reality is that the conditional exported entries are been loaded based on their extensions, not their entry types. With require(esm), it is possible to do the following already 👀:
{
"exports": {
"import": "./index-require.cjs",
"require": "./index-module.mjs"
}
}
|
|
||
| export async function run() { | ||
| // binding.node | ||
| { |
There was a problem hiding this comment.
I personally find describe + it to be worse UX due to the reasons stated in #56027 (comment), unless there is a way to silence the passing tests and make the console logs from the failed test cases appear at the end together with the errors..
|
This needs a rebase. |
legendecas
force-pushed
the
esm/addon
branch
from
7375fa0 to
16777e3
Compare
github-actions
bot
removed
the
request-ci
github-actions
bot
removed
the
request-ci
legendecas
added
commit-queue-squash
github-actions
bot
removed
the
request-ci
legendecas
force-pushed
the
esm/addon
branch
from
d70a87c to
0169b9c
Compare
|
Rebased to address conflicts. |
github-actions
bot
removed
the
request-ci
|
CI is green now, would you mind taking another look at this PR? @nodejs/loaders thanks |
I will do this evening |
aduh95
added
author ready
nodejs-github-bot
removed
the
commit-queue
|
Landed in b6df128 |
Add experimental support to loading
.nodeextension modules in ESM.An addon exports two names
defaultandmodule.exports, as same asimport(cjs)whereits export names can not be preparsed. Addon export names can not be inferred until it is evaluated.
Fixes: #40541
Fixes: #55821